🐛 Add support for build metadata precedence in bundle version comparison #2273

joelanford · 2025-10-16T17:22:08Z

Description

This change fixes an issue to ensure that operator-controller properly handles and compares registry+v1 bundle versions that include build metadata as specified in the semver version.

The intention is that we only treat build metadata as a release value for registry+v1 bundles, which already have this precedent set. If/when operator-controller gains support for new bundle types, the intention is to avoid continuing the practice (and semver violation) of treating version build metadata as comparable/orderable.

Key changes:

Introduce VersionRelease type combining semver version with release metadata
Update bundle comparison logic to consider build metadata when present
Add exact version matching for pinned versions with build metadata
Replace GetVersion with GetVersionAndRelease across the codebase
Ensure successors are determined based on exact version+release matching

This is particularly important for registry+v1 bundles that encode release information in the build metadata field of their version strings.

🤖 Generated with Claude Code

The bug this fixes was originally reported by Red Hat in https://issues.redhat.com/browse/OCPBUGS-60424

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2025-10-16T17:22:18Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`c8e7316`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/69387eefc4e440000732aca6
😎 Deploy Preview	https://deploy-preview-2273--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot

Pull Request Overview

This PR fixes bundle version comparison logic to properly handle registry+v1 bundles that use build metadata in their version strings. The implementation introduces a VersionRelease type that treats build metadata as comparable/orderable release information for registry+v1 bundles (a semver spec violation that exists for backward compatibility). The fix ensures exact version+release matching for pinned versions and successor determination.

Key changes:

Introduced VersionRelease type combining semver version with release metadata parsed from build metadata
Updated comparison and filtering logic to use exact version+release matching instead of semver-only matching
Replaced GetVersion with GetVersionAndRelease throughout the codebase

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
internal/shared/util/slices/slices.go	Added generic Map utility function
internal/operator-controller/resolve/resolver.go	Updated interface to use VersionRelease instead of semver.Version
internal/operator-controller/resolve/catalog.go	Updated to use new comparison functions and moved Map function to shared utilities
internal/operator-controller/controllers/clusterextension_controller.go	Added conversion to legacy registry+v1 version format when creating bundle metadata
internal/operator-controller/catalogmetadata/filter/successors_test.go	Added test case for exact version matching with build metadata
internal/operator-controller/catalogmetadata/filter/successors.go	Implemented exact version+release matching for installed bundle comparison
internal/operator-controller/catalogmetadata/filter/bundle_predicates_test.go	Updated test to use new InSemverRange function
internal/operator-controller/catalogmetadata/filter/bundle_predicates.go	Replaced Masterminds semver with blang semver and added exact version matching
internal/operator-controller/catalogmetadata/compare/compare_test.go	Added tests for NewVersionRange and updated existing tests for new comparison logic
internal/operator-controller/catalogmetadata/compare/compare.go	Implemented NewVersionRange with exact build metadata matching and renamed ByVersion to ByVersionAndRelease
internal/operator-controller/bundleutil/bundle_test.go	Added comprehensive tests for VersionRelease comparison logic
internal/operator-controller/bundleutil/bundle.go	Introduced VersionRelease, Release types and legacy registry+v1 conversion logic

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

internal/operator-controller/catalogmetadata/compare/compare.go

internal/operator-controller/bundleutil/bundle.go

internal/operator-controller/catalogmetadata/compare/compare.go

pedjak · 2025-10-16T22:02:51Z

internal/shared/util/slices/slices.go

@@ -0,0 +1,9 @@
+package slices
+
+func Map[I any, O any](in []I, f func(I) O) []O {


this function is available in https://github.com/samber/lo/blob/master/slice.go#L26 - how about to use it instead?

Not strongly opinionated. I do notice that that would be a new dependency to pull in to the project, so I could go either way.

Anyone else have opinions?

Hearing no other opinions, my vote is to leave as is. With this commit moving this function into a new package in the shared/utils space, hopefully future contributions will find this location to add new generic slice methods, and we can revisit the question again when there is more to be gained from pulling in a new dep.

I'm of two minds on the subject. The reference is to what looks like a personal library, which normally garners a 'no' from me. But it has a mess of contributions, releases, and some kind of community, which relaxes that somewhat.

I'd prefer that this package didn't reuse the same name as a standard library (https://pkg.go.dev/slices).
But otherwise I'm fine with having a minimal utility library here.

But it has a mess of contributions, releases, and some kind of community, which relaxes that somewhat.

The library has 20.5k stars which is really a lot and is a very good signal.

Agreed. I'm wondering why it looks like "some guy", but the support/longevity/utility appears to be there.
From a user perspective, I might be more interested in what dependencies it might bring along, but I see no cause for alarm so far.

codecov · 2025-10-17T04:06:46Z

Codecov Report

❌ Patch coverage is 96.90722% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.32%. Comparing base (f17f3c5) to head (c8e7316).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ator-controller/catalogmetadata/compare/compare.go	94.44%	0 Missing and 1 partial ⚠️
...or-controller/catalogmetadata/filter/successors.go	66.66%	1 Missing ⚠️
internal/operator-controller/resolve/resolver.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2273      +/-   ##
==========================================
+ Coverage   71.10%   71.32%   +0.22%     
==========================================
  Files          95       97       +2     
  Lines        7323     7383      +60     
==========================================
+ Hits         5207     5266      +59     
  Misses       1683     1683              
- Partials      433      434       +1

Flag	Coverage Δ
e2e	`45.14% <68.04%> (+0.11%)`	⬆️
experimental-e2e	`14.33% <0.00%> (?)`
unit	`59.10% <91.75%> (+0.28%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

internal/operator-controller/bundle/versionrelease.go

pedjak · 2025-10-20T08:00:15Z

internal/operator-controller/bundleutil/bundle.go

 )

-func GetVersion(b declcfg.Bundle) (*bsemver.Version, error) {
+func GetVersionAndRelease(b declcfg.Bundle) (*bundle.VersionRelease, error) {


would it make sense to move this function to the new bundle/versionsrelease.go file?

Not strongly opinionated here, but I do I kinda like bundle/versionsrelease.go as a pure implementation of the type without the baggage of the various kinds of things that we might source or derive a VersionRelease from.

internal/operator-controller/bundleutil/bundle_test.go

internal/operator-controller/catalogmetadata/compare/compare.go

pedjak · 2025-10-20T08:29:49Z

internal/operator-controller/catalogmetadata/compare/compare.go

+// ByVersionAndRelease is a comparison function that compares bundles by
+// version and release. Bundles with lower versions/releases are
+// considered less than bundles with higher versions/releases.
+func ByVersionAndRelease(b1, b2 declcfg.Bundle) int {


nit: how about that we implement Compare method on declcfg.Bundle instead?

The reason we don't have that right now is because declcfg.Bundle does not actually embed its own deprecation information. So we can't directly compare declcfg.Bundle along deprecation lines.

I suppose we could essentially combine ByVersionAndRelease and ByDeprecationFunc into a single comparison function, but:

It still wouldn't be a method on declcfg.Bundle

It would still require extra declcfg.Deprecation metadata to do the comparison.

Unfortunately, I've been fighting that concern in operator-registry space as well. I'd like for declcfg.Bundle to have the reusable comparators there, but the historical pattern is that both declcfg and SQLite graph contributions are ultimately converted to Model objects, where final type conversions occur, and validation considers the objects' full scope (including deprecations).

Right now the box feels like "all users must convert to Model objects" to me. 😞

internal/operator-controller/catalogmetadata/filter/bundle_predicates.go

camilamacedo86 · 2025-10-21T07:25:11Z

This one seems related to the bug that @grokspawn has been looking on.
I think it would be nice get his review here.

joelanford · 2025-10-22T18:43:40Z

crd-diff is failing due to the change in the description of the spec.source.catalog.version field. Assuming folks agree to the updated API semantics, we either need to override the failure or update the crd-diff check to ignore the description change.

I'm personally inclined to specifically override so that future description changes continue to be flagged as potentially breaking so that those changes get extra scrutiny.

internal/operator-controller/bundle/versionrelease.go

pedjak

/lgtm a few more functions could be documented.

grokspawn · 2025-10-23T13:39:57Z

...v1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml

+                          metadata (e.g., "1.0.0+20230101"). This ensures an exact match of both
+                          the semver version and the release. If you specify a version without build
+                          metadata (e.g., "1.0.0"), it will match all bundles with that version
+                          regardless of their release information.


We'll need to update this once the bundle release version feature lands, since we will need to mention both approaches, including how one is preferred in future. Right now the bundle release version approach is to use [version]-[release] to express the attribute combo in a string (though the source of truth is always the discrete version and release fields in the olm.package property).

Perhaps this is better resolved in a doc where we can go into more depth?

pedjak

/lgtm

internal/operator-controller/bundle/versionrelease.go

openshift-ci · 2025-10-23T18:04:24Z

New changes are detected. LGTM label has been removed.

Copilot

Pull Request Overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-23T18:04:59Z

internal/operator-controller/catalogmetadata/compare/compare.go

 func compareErrors(err1 error, err2 error) int {
 	if err1 != nil && err2 == nil {
-		return 1
+		return -1


The comment on line 80 states 'The semantic is that errors are "less than" non-errors' but the implementation has been inverted from the original. The comment should be updated to match the new implementation or the implementation should be corrected to match the comment.

camilamacedo86

@joelanford it looks fine for me.
It seems to match with the @grokspawn PEP to solve the issue
Unless I missed something.

Just one thing — I think there’s a scenario we might need to handle in AsLegacyRegistryV1Version.

Also, wdyt about using an AI tool to double-check that we have tests covering all possible cases. This logic is pretty complex, and AI could help make sure we don’t miss any edge cases or weird combos. I think how much more we ensure here with unit test to cover all possible edge cases is better.

camilamacedo86 · 2025-10-28T16:21:36Z

internal/operator-controller/catalogmetadata/filter/successors_test.go

 					Name:     "test-package.v2.1.0",
 					Replaces: "test-package.v2.0.0",
 				},
 				{


@joelanford, I think we can cover a few more cases

Parse a valid semver value with build metadata like 3.0.0-pre+2.beta.1.

Check if we properly deal with invalid builds values (5.0.0+041).

A bunch of those cases are already covered in the operator-registry lib which would be where all these fields are introduced into the FBC.
(for e.g. alpha/property/property_test.go)
I don't think there's a lot of utility in duplicating those tests here.

But the tests that exist in the other project currently will not cover the same logic implemented here.
Ideally, we would have a shared library that provides this functionality so both projects could consume it directly.
However, since the implementation currently lives here and the checks are performed at runtime in this project, we should still have test coverage here. ( and there ; not centralized )

In my opinion, this is important because any change made in this code could break those runtime scenarios, and the tests in the other project would not be able to catch such regressions.

I have all of these cases implemented in internal/operator-controller/bundle/versionrelease_test.go.

camilamacedo86 · 2025-10-28T16:23:53Z

internal/operator-controller/catalogmetadata/compare/compare_test.go


 	"github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/compare"
 )



@joelanford wdy about use AI here to check if we got tests for all these?
this logic is kinda complex and I feel maybe some cases missing?
Are we covering things like:

order tests (empty, number, text, mix, different length)

pin == (same version but diff release)

wdyt? just to be safe we cover everything

I found two more cases to cover:

An invalid range returns an error

A range that does not include the version returns false.

Adding these.

camilamacedo86 · 2025-10-28T16:33:47Z

internal/operator-controller/bundle/versionrelease.go

+		Pre:   vr.Version.Pre,
+		Build: slicesutil.Map(vr.Release, func(i bsemver.PRVersion) string { return i.String() }),
+	}
+}


@joelanford it might have a small edge case here.
What happens if the build metadata can’t be parsed as a Release?
I think if parsing failed and Release is empty, we’d end up losing the original build metadata. It would be nice have a test to cover this scenario either.

Maybe related: #2273 (comment)

Yeah, I caught that as well. That code has been updated to return a BundleVersion where Version contains version+buildmetadata and no Release

I've also updated AsLegacyRegistryV1Version to make sure it correctly handles a "releaseless" BundleVersion.

This change fixes an issue to ensure that operator-controller properly handles and compares registry+v1 bundle versions that include build metadata as specified in the semver version. The intention is that we only treat build metadata as a release value for registry+v1 bundles, which already have this precedent set. If/when operator-controller gains support for new bundle types, the intention is to avoid continuing the practice (and semver violation) of treating version build metadata as comparable/orderable. Key changes: - Introduce VersionRelease type combining semver version with release metadata - Update bundle comparison logic to consider build metadata when present - Add exact version matching for pinned versions with build metadata - Replace GetVersion with GetVersionAndRelease across the codebase - Ensure successors are determined based on exact version+release matching This is particularly important for registry+v1 bundles that encode release information in the build metadata field of their version strings. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

…rove VersionRelease parsing This commit reverts the user-facing semantic changes to the ClusterExtension version field that were introduced to support exact version pinning with build metadata. The version field now ignores build metadata when matching versions, consistent with semver specification. Additionally, this commit modifies the VersionRelease parsing logic to be more tolerant of semver versions whose build metadata is not a valid release. When build metadata cannot be parsed as a release, the full version (including build metadata) is preserved in the Version field, with an empty Release field. Changes include: - Removed documentation about pinning to exact versions with build metadata - Removed exactVersionMatcher logic that enforced build metadata equality - Updated NewLegacyRegistryV1VersionRelease to tolerate non-release build metadata - Updated test expectations to reflect new behavior

openshift-ci · 2025-12-09T14:59:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pedjak

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [pedjak]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

internal/operator-controller/bundleutil/bundle_test.go:92

The test doesn't verify the expected wantVersionRelease value for test cases where wantErr is false. Consider adding an assertion to check that the returned value matches tc.wantVersionRelease when no error is expected:

vr, err := bundleutil.GetVersionAndRelease(bundle)
if tc.wantErr {
    require.Error(t, err)
} else {
    require.NoError(t, err)
    require.Equal(t, tc.wantVersionRelease, vr)
}

			_, err := bundleutil.GetVersionAndRelease(bundle)
			if tc.wantErr {
				require.Error(t, err)
			} else {
				require.NoError(t, err)
			}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Add documentation for AsLegacyRegistryV1Version method explaining the build metadata conversion logic - Fix AsLegacyRegistryV1Version to preserve original build metadata when Release field is not set - Add comprehensive test coverage for NewLegacyRegistryV1VersionRelease including edge cases for build metadata and release parsing - Add test coverage for AsLegacyRegistryV1Version conversion logic - Improve compare_test.go test structure with descriptive test names and assertion functions for better clarity - Add test case for non-release build metadata comparison

joelanford requested a review from a team as a code owner October 16, 2025 17:22

joelanford requested review from Copilot and removed request for a team October 16, 2025 17:22

openshift-ci bot requested review from anik120 and dtfranz October 16, 2025 17:22

Copilot AI reviewed Oct 16, 2025

View reviewed changes

internal/operator-controller/catalogmetadata/compare/compare.go Show resolved Hide resolved

joelanford force-pushed the build-metadata-precedence branch 3 times, most recently from c2cd023 to d352bc0 Compare October 16, 2025 18:24

pedjak reviewed Oct 16, 2025

View reviewed changes

joelanford force-pushed the build-metadata-precedence branch from d352bc0 to e8571b6 Compare October 17, 2025 03:54

joelanford force-pushed the build-metadata-precedence branch from e8571b6 to b398c34 Compare October 17, 2025 17:24

pedjak reviewed Oct 20, 2025

View reviewed changes

joelanford force-pushed the build-metadata-precedence branch from b398c34 to 2cf7680 Compare October 22, 2025 18:05

pedjak reviewed Oct 23, 2025

View reviewed changes

internal/operator-controller/bundle/versionrelease.go Show resolved Hide resolved

pedjak reviewed Oct 23, 2025

View reviewed changes

internal/operator-controller/bundle/versionrelease.go Show resolved Hide resolved

pedjak approved these changes Oct 23, 2025

View reviewed changes

openshift-ci bot assigned pedjak Oct 23, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2025

grokspawn reviewed Oct 23, 2025

View reviewed changes

joelanford force-pushed the build-metadata-precedence branch from 2cf7680 to 72c3a34 Compare October 23, 2025 14:45

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2025

pedjak approved these changes Oct 23, 2025

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2025

joelanford commented Oct 23, 2025

View reviewed changes

internal/operator-controller/bundle/versionrelease.go Show resolved Hide resolved

Copilot AI review requested due to automatic review settings October 23, 2025 18:04

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2025

Copilot AI reviewed Oct 23, 2025

View reviewed changes

camilamacedo86 reviewed Oct 28, 2025

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2025

joelanford and others added 2 commits December 9, 2025 09:57

joelanford force-pushed the build-metadata-precedence branch from 6372625 to fb24d4e Compare December 9, 2025 14:59

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 9, 2025

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2025

Copilot AI review requested due to automatic review settings December 9, 2025 16:10

Copilot started reviewing on behalf of joelanford December 9, 2025 16:10 View session

Copilot AI reviewed Dec 9, 2025

View reviewed changes

joelanford force-pushed the build-metadata-precedence branch from 7025aa5 to c8e7316 Compare December 9, 2025 19:56

		@@ -0,0 +1,9 @@
		package slices

		func Map[I any, O any](in []I, f func(I) O) []O {


		"github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/compare"
		)

🐛 Add support for build metadata precedence in bundle version comparison #2273

Are you sure you want to change the base?

🐛 Add support for build metadata precedence in bundle version comparison #2273

Uh oh!

Conversation

joelanford commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer Checklist

Uh oh!

netlify bot commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

camilamacedo86 commented Oct 21, 2025

Uh oh!

joelanford commented Oct 22, 2025

Uh oh!

Uh oh!

Uh oh!

pedjak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedjak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openshift-ci bot commented Oct 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

joelanford commented Oct 16, 2025 •

edited

Loading

netlify bot commented Oct 16, 2025 •

edited

Loading

joelanford Oct 17, 2025 •

edited

Loading

codecov bot commented Oct 17, 2025 •

edited

Loading

camilamacedo86 Oct 30, 2025 •

edited

Loading